Refactor(packages/codemode/src/stdlib/value.ts): reduce complexity in coerceToString and invokeCoercion - #69
Open
rymameuri wants to merge 2 commits into
Open
Conversation
…rcion Replace instanceof/name if-chains with lookup tables to cut function complexity and return-path count flagged by qlty.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
P1B: Starter Task: Refactoring PR
1. Issue
Link to the associated GitHub issue: Closes #67
Full path to the refactored file: packages/codemode/src/stdlib/value.ts
What do you think this file does?
This file implements the CodeMode sandbox's type-coercion helpers, it converts sandboxed values (Date, RegExp, Map, Set, URL, arrays, etc.) into strings and numbers, and it's what the sandbox's exposed Number/String/Boolean/parseInt/parseFloat global functions call into when user code invokes them.
What is the scope of your refactoring within that file?
coerceToString and invokeCoercion, plus two small helper functions extracted from them during the refactor, coerceArrayToString and coerceParseInt.
Which Qlty-reported issue did you address?
"Function with many returns" (count = 10) and "Function with high complexity" on both functions ,coerceToString at complexity 30 and invokeCoercion at complexity 26, with total file complexity of 65 before the change.
2. Refactoring
How did the specific issue you chose impact the codebase's maintainability?
Both functions had grown into long chains of sequential if/return statements dispatching on a value's type (coerceToString) or a function name string (invokeCoercion), which made each function hard to scan and meant adding any new case meant adding yet another branch to an already-long chain.
What changes did you make to resolve the issue?
I replaced the sequential instanceof/name checks with lookup tables (sandboxStringFormatters for coerceToString; sandboxCoercions and valueCoercions for invokeCoercion) resolved with a single lookup, and extracted the array-to-string logic and the parseInt radix-validation logic into their own named helper functions.
How do your changes improve maintainability? Did you consider alternatives?
Adding a new coercion case is now one table entry instead of another if branch, and complexity dropped because dispatch happens through a single lookup rather than N sequential conditionals; I considered splitting each function into several smaller functions along the existing if/else boundaries instead, but a lookup table was clearer since every branch was doing the same dispatch-by-key work.
3. Validation
How did you validate that the change is correct?
I ran qlty smells on the file before and after the refactor, ran bun run lint scoped to the file, and ran bun test for the codemode package; I also added two tests (covering array-to-string coercion and parseInt's invalid-radix error path) so both changed functions, including the extracted helpers, are exercised by passing tests, not just covered incidentally by the rest of the suite.
IMAGE PROOF
(these two show qlty smells --no-snippets packages/codemode/src/stdlib/value.ts before/after)
(0 warnings, 0 errors — bun run lint scoped to the changed file)
(263 pass, 0 fail — bun test for the codemode package)
(98.61% lines / 77.27% funcs on value.ts, no uncovered lines, 105 pass/0 fail — covers both the coverage screenshot and the "tests that cover the change passing" screenshot the template asks for)
Github checks